RBS: Narrow type signatures for props parameters

[justin808]

Summary

Improves type precision for props parameters in RBS signatures by changing from untyped to Hash[Symbol, untyped] | String.

Changes

• helper.rbs: Updated sanitized_props_string props parameter type
• controller.rbs: Updated redux_store props parameter type

Benefits

• More precise type checking for props parameters
• Accepts both common formats (Hash or JSON string)
• Better IDE support and autocomplete
• Catches type errors earlier

Testing

• ✅ bundle exec rubocop passes with no violations
• ✅ Pre-commit hooks pass (formatting, linting, trailing newlines)

Fixes #1951

🤖 Generated with Claude Code

---

This change is 

[coderabbitai]

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 18 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 9404439 and 610fdd3.

📒 Files selected for processing (2)

• sig/react_on_rails/controller.rbs (1 hunks)
• sig/react_on_rails/helper.rbs (2 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/narrow-rbs-props-types

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review for PR #1966

Summary

This PR improves RBS type precision for props parameters by narrowing the type from untyped to Hash[Symbol, untyped] | String. The changes are well-focused and address issue #1951.

---

✅ Positives

1. Type Safety Improvement: The new type signature accurately reflects the actual implementation in helper.rb:190-191 and controller.rb:17, where props are checked with props.is_a?(String) and handled accordingly.

2. Consistent with Runtime Behavior: The implementation shows props can be either:

   • A Ruby Hash (converted via .to_json)
   • A JSON String (used directly)

   The RBS signatures now correctly model this.

3. Minimal, Focused Change: Only 2 lines changed across 2 files - exactly what's needed, no scope creep.

4. Good Test Coverage: The existing tests in spec/dummy/spec/helpers/react_on_rails_helper_spec.rb:178-192 already validate both Hash and String inputs to sanitized_props_string.

5. Follows Project Standards: PR includes proper testing checklist and follows the CLAUDE.md guidelines.

---

🔍 Considerations & Questions

1. Hash Key Type - Minor Type Accuracy Issue:

   The signature uses Hash[Symbol, untyped], but looking at the actual usage patterns:

   • In tests (helper_spec.rb:52-58), hashes use symbol keys: { hello: "world" }
   • However, Rails often converts these to string keys in JSON contexts
   • The props_string helper (helper.rb:512-514) calls .to_json which could handle both

   Question: Should this be Hash[Symbol | String, untyped] to be more accurate, or is Hash[Symbol, untyped] the documented convention for the API?

2. Alternative Type Consideration:

   Could also consider using a type alias for reusability:

   type props = Hash[Symbol, untyped] | String

   But this may be overkill for 2 usages.

---

🧪 Testing Assessment

Coverage: ✅ Excellent

Existing test coverage validates the type changes:

• sanitized_props_string tests both Hash and String inputs (lines 178-192)
• redux_store tests use Hash props (line 524)
• Real usage examples throughout the codebase show both patterns

Recommendation: No new tests needed - existing coverage is comprehensive.

---

🔒 Security

✅ No security concerns. The change only affects type checking, not runtime behavior.

---

⚡ Performance

✅ Zero performance impact - RBS signatures are for static analysis only.

---

📝 Documentation

Suggestion: Consider adding a brief comment in the RBS files explaining the dual Hash/String acceptance:

# Props can be a Hash (converted to JSON) or pre-serialized JSON String
def sanitized_props_string: (Hash[Symbol, untyped] | String props) -> String

Though this is optional, as the implementation is self-documenting.

---

✅ Final Verdict

LGTM with one minor question about the Hash key type. This is a solid improvement that makes the type signatures more precise while accurately reflecting the runtime behavior.

Recommendation: ✅ Approve after clarifying the Hash key type question (Symbol vs Symbol | String)

Great work on improving type safety! 🎉

[claude]

Code Review - PR #1966: RBS Type Signature Improvements

Summary

This PR successfully narrows the type signatures for props parameters from untyped to Hash[Symbol, untyped] | String, improving type safety and developer experience. The changes are well-scoped and aligned with the actual implementation.

---

✅ Strengths

1. Accurate Type Signatures

• The new signatures correctly reflect the runtime behavior in lib/react_on_rails/helper.rb:190-191 and lib/react_on_rails/controller.rb:17
• Both methods handle props as either a Hash or String (JSON), exactly as the type signature now indicates
• Test coverage in spec/dummy/spec/helpers/react_on_rails_helper_spec.rb confirms both Hash and String inputs are valid (lines 179-192, 232-244)

2. Improved Developer Experience

• Better IDE autocomplete and type hints
• More precise error messages when incorrect types are passed
• Maintains backward compatibility while adding type safety

3. Clean Implementation

• Minimal, focused changes (2 additions, 2 deletions)
• No runtime code changes required
• All CI checks passing

---

🔍 Technical Analysis

Type Accuracy:
The implementation uses duck typing via props.is_a?(String) checks, which means:

• Hash[Symbol, untyped] matches standard Ruby hashes with symbol keys
• String matches JSON string representations
• The to_json method is called on non-String values, which works for Hash, HashWithIndifferentAccess, and any object responding to to_json

Potential Edge Case:
While HashWithIndifferentAccess is commonly used in Rails and works correctly at runtime (as evidenced by test at line 145-147 in helper_spec.rb), the RBS signature only explicitly mentions Hash[Symbol, untyped]. This is acceptable because:

1. HashWithIndifferentAccess responds to to_json
2. The runtime check props.is_a?(String) will be false, triggering the to_json path
3. RBS doesn't need to list every possible hash-like type

---

📋 Best Practices Compliance

Per CLAUDE.md requirements:

• ✅ RuboCop compliance verified (mentioned in PR description)
• ✅ Type signatures match actual implementation
• ✅ No formatting issues (RBS files don't require Prettier)
• ✅ Fixes referenced issue RBS: Narrow type signatures for props parameters #1951

---

💡 Suggestions (Optional)

1. Consider Adding RBS Comment Documentation
While not blocking, adding RBS comments could help future maintainers:

# Props can be either:
# - A Hash with Symbol keys (common case)
# - A JSON string representation (for pre-serialized data)
def sanitized_props_string: (Hash[Symbol, untyped] | String props) -> String

2. Future Enhancement: Union Type for Controller Props
The controller method at lib/react_on_rails/controller.rb:17 defaults to {}, which is consistent with the type signature. This is correct as-is.

---

🎯 Verdict

Recommendation: ✅ APPROVE

This is a high-quality, low-risk improvement that:

• Correctly narrows overly-broad type signatures
• Maintains backward compatibility
• Improves type safety and DX
• Follows all project conventions
• Has comprehensive test coverage

No changes required. Ready to merge.

---

📚 References

• Implementation: lib/react_on_rails/helper.rb:190-191
• Implementation: lib/react_on_rails/controller.rb:17
• Tests: spec/dummy/spec/helpers/react_on_rails_helper_spec.rb:178-193, 232-244
• Related: Issue RBS: Narrow type signatures for props parameters #1951, PR Add RBS type signatures for improved type safety #1945

[claude]

Code Review: RBS Type Signature Improvements

Summary

This PR narrows the type signatures for props parameters from untyped to Hash[Symbol, untyped] | String, which is a good improvement for type safety.

✅ Strengths

1. Accurate Type Signatures: The changes correctly reflect the actual implementation:

   • sanitized_props_string at helper.rb:190-191 handles both Hash and String
   • redux_store methods accept Hash or String props per documentation (helper.rb:43, controller.rb:10)
   • Implementation uses .is_a?(String) checks confirming the union type

2. Consistent with Documentation: Comments in helper.rb:43 explicitly state "Ruby Hash or JSON string"

3. Good Test Coverage: Existing tests at react_on_rails_helper_spec.rb:185-199 verify both Hash and String inputs work correctly

4. Follows Best Practices:

   • Small, focused change addressing a specific issue (RBS: Narrow type signatures for props parameters #1951)
   • Pre-commit hooks passed
   • RuboCop clean

🔍 Observations

1. Type Precision: While Hash[Symbol, untyped] is used, the actual implementation in props_string (helper.rb:512-514) and sanitized_props_string (helper.rb:190-191) don't explicitly enforce Symbol keys - they just call .to_json. This is fine as Ruby hashes can have Symbol keys and to_json handles them, but worth noting.

2. Helper vs Controller Discrepancy: There's an existing discrepancy in the RBS files:

   • helper.rbs:22-25: redux_store has ?Hash[Symbol, untyped] props (not a union type yet)
   • controller.rbs:6-10: This PR updates to ?props: Hash[Symbol, untyped] | String

   The helper.rbs version should likely be updated in a follow-up or this PR to maintain consistency, since both implementations support the same types.

3. Nil Handling: Both methods default to {} when props is not provided, which is correctly captured by the optional parameter syntax.

📝 Recommendations

Minor Issue: Consider updating helper.rbs line 22-25 to match the type signature improvement:

def redux_store: (
  String store_name,
  ?props: Hash[Symbol, untyped] | String,
  ?defer: bool,
  ?immediate_hydration: bool
) -> safe_buffer

Currently it's still typed as just Hash[Symbol, untyped] without the union with String.

✅ Verdict

Approved with minor suggestion. The changes are correct and improve type safety. The only improvement would be ensuring consistency across all redux_store signatures in the RBS files.

---

Test Coverage: ✅ Existing tests verify both Hash and String inputs
Type Accuracy: ✅ Matches actual implementation behavior
Documentation: ✅ Aligns with code comments
CI/Linting: ✅ Passes per PR description

[claude]

PR Review: RBS Type Signature Improvements for Props Parameters

Summary

This PR successfully narrows the type signatures for props parameters from untyped to Hash[Symbol, untyped] | String in the RBS files, providing better type safety while maintaining backward compatibility.

✅ Strengths

1. Accurate Type Signatures: The changes correctly reflect the actual implementation:

   • helper.rb:191: sanitized_props_string checks props.is_a?(String) and handles both Hash and String
   • helper.rb:513: props_string method also handles both String and Hash types
   • helper.rb:160 & controller.rb:17: redux_store methods accept props that are passed through these utility methods

2. Consistency: All three updated signatures (redux_store in helper.rbs, redux_store in controller.rbs, and sanitized_props_string) now consistently use the same type union.

3. Test Coverage: The existing test suite already validates both use cases:

   • Hash props: spec/dummy/spec/helpers/react_on_rails_helper_spec.rb:528-530 tests with Hash
   • String props: spec/dummy/spec/helpers/react_on_rails_helper_spec.rb:234-247 tests with JSON string

4. Documentation Alignment: The comments in helper.rb:43-44 and helper.rb:155 confirm props can be "Ruby Hash or JSON string", which matches the new signatures.

🔍 Observations

1. Type Precision Trade-off: While Hash[Symbol, untyped] is specified, Ruby Hashes with string keys or HashWithIndifferentAccess would technically work at runtime since they're converted to JSON via .to_json. However, this stricter typing encourages the idiomatic Ruby pattern of using symbol keys, which is reasonable.

2. RBS Best Practice: The union type Hash[Symbol, untyped] | String is properly ordered (more specific type first) and follows RBS conventions.

🎯 Recommendations

None - this PR is ready to merge! The changes are:

• ✅ Accurate and safe
• ✅ Well-tested by existing specs
• ✅ Properly documented
• ✅ Consistent across all affected signatures
• ✅ Follows RBS best practices

📋 Testing Notes

Per the PR description, the mandatory checks have passed:

• ✅ bundle exec rubocop passes with no violations
• ✅ Pre-commit hooks pass (formatting, linting, trailing newlines)

🎉 Conclusion

This is a clean, straightforward improvement that enhances type safety without breaking existing functionality. The type signatures now accurately document the API contract and will provide better IDE support and earlier error detection for users of the gem.

Recommendation: Approve and merge ✅

---

Review generated with Claude Code