Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

Copy link
Member Author

NathanFlurry commented Jan 24, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Jan 24, 2026

PR Review: feat(rivetkit): add icon & name customization

Overview

This PR adds the ability to customize actor display names and icons in the Rivet Inspector and dashboard UI. The implementation includes support for both emoji and FontAwesome icons, with a well-designed fallback system and integration with the existing workflow system.

Code Quality & Best Practices

✅ Strengths

  1. Clean API Design: The dual approach (options-level and run-level metadata) is well thought out, with clear precedence rules (options override run metadata).

  2. Good Type Safety: Proper use of Zod schemas and TypeScript types throughout, maintaining the existing type-safety standards.

  3. Excellent Documentation: The new appearance.mdx documentation is comprehensive, well-structured, and includes practical examples for both basic users and library developers.

  4. Backward Compatibility: The changes are fully backward compatible - existing actors without icon/name configuration continue to work with sensible defaults.

  5. Integration with Existing Patterns: The workflow() helper already returns a RunConfig with an icon, demonstrating the pattern works well with existing code.

⚠️ Issues & Concerns

1. Emoji Regex Pattern Incomplete (Medium Priority)

Location: frontend/src/app/actor-builds-list.tsx:12-13

The emoji regex pattern is incomplete and will miss many modern emojis. It's missing:

  • Emoji modifiers (skin tones)
  • Variation selectors
  • Zero-width joiners (ZWJ) sequences used in modern emojis (family emojis, profession emojis, etc.)
  • Newer emoji ranges

Example failures: Family emoji with ZWJ, thumbs up with skin tone, and many Unicode 13.0+ emojis would fail.

Recommendation: Consider using a well-tested emoji detection library or a more comprehensive regex pattern.

2. useMemo Hook in Loop (High Priority - Bug)

Location: frontend/src/app/actor-builds-list.tsx:60-76

This useMemo is inside the .map() callback (line 50), which violates React's Rules of Hooks. Hooks cannot be called conditionally or in loops.

Impact: This will cause React errors and unpredictable behavior.

Fix: Remove the useMemo entirely - it's premature optimization here. The icon lookup is cheap and happens during render anyway. Just compute the iconElement directly in the map callback.

3. Case Sensitivity in Icon Lookup (Low Priority)

Location: frontend/src/app/actor-builds-list.tsx:30-34

If a user provides case variations like "ChartLine" or "CHART-LINE" instead of "chart-line", the lookup will fail silently. Consider normalizing the input to lowercase before processing.

4. Type Safety with Unknown Values (Low Priority)

Location: rivetkit-typescript/packages/rivetkit/src/registry/config/index.ts:275-282

Using Record<string, unknown> and then assigning potentially undefined values is a bit loose. Consider being more explicit with the type or using conditional assignment.

Performance Considerations

✅ Good

  • Icon lookup is appropriately lightweight (simple object property access)
  • Metadata extraction happens at registry build time, not at runtime
  • The helper utilities are simple type guards with no heavy computation

⚠️ Minor Concern

  • The glob import might import more than needed, but this is likely acceptable for an icon library.

Security Concerns

✅ No Major Issues

  • User-provided strings are only used for display and property lookups
  • No XSS risk - React handles emoji/text rendering safely
  • Invalid FontAwesome icon names fail gracefully with fallback to default icon

Test Coverage

Missing Tests (High Priority)

No tests were added for this feature. Critical areas that need testing:

  1. Unit tests for helper functions:

    • isEmoji() with various emoji formats
    • toPascalCase() with different inputs
    • lookupFaIcon() with valid/invalid icon names
    • getRunFunction() and getRunMetadata() with different input types
  2. Integration tests:

    • Actor registration with icon/name in options
    • Actor registration with RunConfig from run handler
    • Precedence: options override run metadata
    • Default behavior when icon/name not provided

Architecture & Design

✅ Well-Designed

  • Clear separation of concerns: config parsing, metadata extraction, UI rendering
  • Follows existing patterns (Zod schemas, type exports, helper functions)
  • The RunConfig pattern is extensible for future metadata needs

💡 Suggestion

Consider documenting the precedence order more explicitly in code comments, especially in buildActorNames().

Documentation

✅ Excellent

  • Comprehensive new doc page with clear examples
  • Covers both basic usage and advanced library developer patterns
  • Good integration with existing actor docs
  • Sitemap updated appropriately

💡 Minor Suggestions

  1. Consider adding a note about which FontAwesome icon sets are available
  2. Add a troubleshooting section for "icon not showing" scenarios

Summary

Must Fix (Before Merge)

  1. Fix React Hooks violation - Remove useMemo from inside .map() callback
  2. Add test coverage - At minimum, unit tests for helper functions and integration tests for metadata extraction

Should Fix (High Priority)

  1. Improve emoji regex - Use a more comprehensive pattern or library
  2. Normalize icon names - Make FontAwesome lookup case-insensitive

Nice to Have (Low Priority)

  1. Improve type safety in buildActorNames()
  2. Add troubleshooting docs

Conclusion

This is a well-designed feature with excellent documentation and clean API design. However, there's a critical React Hooks violation that must be fixed before merging, and the lack of tests is concerning for a user-facing feature. Once these issues are addressed, this will be a solid addition to RivetKit.

Recommendation: Request changes for the Hooks violation and test coverage, then approve after those are addressed.

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