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.

@NathanFlurry NathanFlurry mentioned this pull request Jan 24, 2026
@claude
Copy link

claude bot commented Jan 24, 2026

PR Review: Icon & Name Customization

Thanks for this feature! The ability to customize actor appearance in the Inspector is a great developer experience improvement. Overall, the implementation is solid and well-documented. Here's my feedback:


✅ Strengths

  1. Excellent Documentation: The new appearance.mdx page is comprehensive with clear examples and explains both basic usage and the advanced RunConfig pattern for library authors.

  2. Clean Architecture: The precedence system (actor options > run metadata) is logical and well-implemented in registry/config/index.ts:267-284.

  3. Type Safety: Good use of Zod schemas and TypeScript types throughout. The RunConfigSchema properly validates the structure.

  4. Helper Functions: getRunFunction and getRunMetadata provide clean separation of concerns in actor/config.ts:59-74.


🔍 Issues & Suggestions

1. Performance Issue: useMemo Dependencies in Frontend ⚠️

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

The iconElement is memoized but depends on iconValue, which is calculated outside the useMemo. The computation is already very cheap (emoji regex test + object lookup), so useMemo adds unnecessary complexity here.

Recommendation: Remove useMemo entirely - the computation is already very cheap:

const iconValue = typeof build.name.metadata.icon === "string" ? build.name.metadata.icon : null;
const displayName = typeof build.name.metadata.name === "string" ? build.name.metadata.name : build.id;

let iconElement;
if (iconValue && isEmoji(iconValue)) {
  iconElement = <span className="...">{iconValue}</span>;
} else {
  const faIcon = iconValue ? lookupFaIcon(iconValue) : null;
  iconElement = <Icon icon={faIcon ?? faActorsBorderless} className="..." />;
}

2. Incomplete Emoji Regex ⚠️

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

The current regex misses many valid emojis:

  • Missing ranges like supplemental symbols, extended pictographs
  • Doesn't handle compound emojis with ZWJ sequences (👨‍👩‍👧‍👦)
  • Doesn't handle skin tone modifiers (👋🏽)

Recommendation: Use a more comprehensive pattern or the emoji-regex npm package for production-grade emoji detection.


3. Case Sensitivity in FontAwesome Lookup 💡

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

The conversion assumes kebab-case input (e.g., "chart-line""faChartLine"), but doesn't handle variations like camelCase or snake_case.

Recommendation: Document the expected format (kebab-case) clearly in the docs, or add input normalization to handle common variations.


4. Type Safety: any in Helper Functions 💡

Location: rivetkit-typescript/packages/rivetkit/src/actor/config.ts:59-74

Using any for function arguments/return types loses type safety. Consider using unknown or generic constraints.


5. Missing Error Handling for Invalid Icons 💡

Current behavior: Falls back to faActorsBorderless silently.

Recommendation: Consider adding development warnings when invalid icon names are provided to help developers debug configuration issues.


6. Documentation: Missing Frontmatter 📝

Location: website/src/content/docs/actors/appearance.mdx:1

According to CLAUDE.md, docs require frontmatter with title, description, and skill fields. This file is missing frontmatter entirely.

Required:

---
title: "Icons & Names"
description: "Customize actor appearance with display names and icons in the Rivet Inspector"
skill: false
---

7. Code Style: Comment Format 📝

Location: rivetkit-typescript/packages/rivetkit/src/actor/config.ts:45-52

Minor inconsistency: Mixing regular comments (//) with JSDoc comments (/** */). Use JSDoc for all exported types/functions for better IDE integration.


🧪 Testing Suggestions

No tests were added for this feature. Consider adding:

  1. Unit tests for helper functions:

    • Test isEmoji with various emoji types
    • Test toPascalCase with edge cases
    • Test lookupFaIcon with valid/invalid icon names
  2. Integration tests:

    • Verify buildActorNames correctly merges options and run metadata
    • Test precedence: actor options should override run metadata
  3. Frontend tests:

    • Verify emoji and FontAwesome icons render correctly
    • Verify fallback behavior

🔒 Security

No security concerns identified. The icon values are used only for UI rendering and don't pose XSS risks since React escapes strings automatically.


📊 Summary

Category Status
Code Quality ⚠️ Minor issues with useMemo usage
Bugs ⚠️ Emoji regex incomplete, may miss valid emojis
Performance ⚠️ Unnecessary complexity from useMemo
Security ✅ No concerns
Testing ⚠️ No tests added
Documentation ⚠️ Missing frontmatter, otherwise excellent

🎯 Recommendations Priority

Must fix before merge:

  1. Add frontmatter to appearance.mdx

Should fix:

  1. Fix useMemo usage in actor-builds-list.tsx (remove it)
  2. Improve emoji regex or use a library
  3. Add tests for helper functions

Nice to have:

  1. Add development warnings for invalid icon names
  2. Improve type safety (any → unknown/generics)
  3. Document expected icon name format (kebab-case)

Great work overall! This is a valuable UX improvement. Let me know if you'd like me to help with any of these issues.

@NathanFlurry NathanFlurry force-pushed the 01-24-feat_rivetkit_add_icon_name_customization branch from 503ae21 to a3e52db Compare January 28, 2026 01:12
@NathanFlurry NathanFlurry force-pushed the 01-24-feat_rivetkit_add_icon_name_customization branch from a3e52db to d5e770c Compare January 30, 2026 08:19
@claude
Copy link

claude bot commented Jan 30, 2026

PR Review: Icon & Name Customization for Rivet Actors

Overview

This PR adds the ability to customize actor display names and icons in the Rivet inspector and dashboard. The implementation is clean and follows a layered approach with good defaults and override mechanisms.


✅ Strengths

1. Well-Designed API

  • Clear precedence hierarchy: options.name/icon > run.name/icon > defaults
  • Flexible icon format (emoji or FontAwesome names)
  • Backward compatible - all properties are optional

2. Clean Implementation

  • Helper functions (getRunFunction, getRunMetadata) properly separate concerns
  • Zod schemas correctly validate the new fields
  • Frontend code cleanly handles both emoji and FontAwesome icons

3. Excellent Documentation

  • Comprehensive new docs page (appearance.mdx) with multiple examples
  • Clear explanation of precedence rules
  • Good coverage of both basic and advanced use cases

🐛 Issues Found

CRITICAL: React Hook Dependency Issue

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

Problem: useMemo is called inside a .map() callback, violating React's Rules of Hooks. Hooks must be called at the top level of a component, not inside loops.

Solution: Remove the useMemo wrapper. The computation is simple (emoji check + icon lookup) and doesn't need memoization. Calculate the icon element directly without useMemo.


🔍 Minor Issues & Suggestions

1. Emoji Regex Coverage

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

Concern: The regex covers many emoji ranges but may miss some valid emojis (e.g., skin tone modifiers, ZWJ sequences). Consider using a more robust library like emoji-regex for production, or document the limitations.

2. Type Safety in Frontend

Suggestion: Add TypeScript types for the build metadata structure to avoid runtime type checks. This would catch type mismatches at compile time.

3. Error Handling for Invalid FontAwesome Icons

Suggestion: Consider logging a warning (in dev mode) when an icon name doesn't resolve. This helps developers catch typos like "chart-lines" instead of "chart-line".

4. Inconsistent Metadata Cleanup

Location: rivetkit-typescript/packages/rivetkit/src/registry/config/index.ts:279-281

Consider whether you want to delete falsy values ("", 0, false) or just undefined/null. Currently uses !metadata.icon which would delete empty strings, but you may want === undefined instead for more precise control.


🧪 Testing Recommendations

Unit Tests Needed:

  • Test getRunFunction() with function, RunConfig object, and undefined
  • Test getRunMetadata() extraction logic
  • Test buildActorNames() precedence rules
  • Test emoji detection regex with various emoji types
  • Test lookupFaIcon() with valid/invalid icon names

Integration Tests:

  • Verify actors with options.icon display correctly in inspector
  • Verify actors with run.icon display correctly
  • Verify precedence rules (options override run metadata)
  • Test both emoji and FontAwesome rendering

Edge Cases:

  • Empty string for name/icon
  • Invalid FontAwesome icon names
  • Non-BMP emoji characters
  • Very long display names

🔒 Security Considerations

No major security concerns identified. The implementation uses type-safe string values with no XSS risk.


📊 Performance Considerations

Good overall. Icon lookup on every render is lightweight (simple object access). The useMemo removal actually improves performance by avoiding hook overhead.


🎯 Code Quality & Best Practices

Follows Repository Standards ✅

  • TypeScript types properly defined
  • Zod schemas for runtime validation
  • Documentation follows existing patterns
  • Sitemap properly updated

Violates Repository Standards ❌

  • React Hooks Rules: useMemo inside loop (MUST FIX)

📝 Summary

This is a well-designed feature with excellent documentation, but has one critical bug that must be fixed before merging.

Required Changes:

  1. BLOCKER: Remove useMemo from inside the map callback (violates Rules of Hooks)

Recommended Changes:

  1. Add unit tests for helper functions
  2. Consider more robust emoji detection or document limitations
  3. Add dev-mode warnings for invalid FontAwesome icon names
  4. Refine metadata cleanup logic to handle edge cases

Nice-to-Have:

  1. Move utility functions to separate file for reusability
  2. Add TypeScript types for build metadata structure
  3. Integration test for icon display in inspector

Overall Assessment: Strong implementation of a useful feature. Fix the React hook issue and this will be ready to merge! 🚀

@graphite-app
Copy link
Contributor

graphite-app bot commented Feb 4, 2026

Merge activity

  • Feb 4, 8:36 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Feb 4, 8:37 PM UTC: CI is running for this pull request on a draft pull request (#4114) due to your merge queue CI optimization settings.
  • Feb 4, 8:38 PM UTC: Merged by the Graphite merge queue via draft PR: #4114.

@graphite-app graphite-app bot closed this Feb 4, 2026
@graphite-app graphite-app bot deleted the 01-24-feat_rivetkit_add_icon_name_customization branch February 4, 2026 20:38
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.

1 participant